Enable setting condition on Prometheus Collector #5317

BenB196 · 2023-02-18T16:18:18Z

Enhancement

What does this PR do?

In the Prometheus Collector, switch from condition being a toggle for Leader Election only, to a text field.

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

I'm not sure what the policy for this type of change is, but when upgrading existing Integrations via this change, it will blank out an existing leader election condition, if one was previously set:

Integration Before Upgrade

inputs:
  - id: prometheus/metrics-prometheus-eb767fbc-1e20-47da-b85d-973f5b4195f2
    name: prometheus-1
    revision: 1
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.0
    data_stream:
      namespace: default
    package_policy_id: eb767fbc-1e20-47da-b85d-973f5b4195f2
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-eb767fbc-1e20-47da-b85d-973f5b4195f2
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metricsets:
          - collector
        hosts:
          - 'localhost:9090'
        metrics_filters.exclude: null
        metrics_filters.include: null
        metrics_path: /metrics
        period: 10s
        rate_counters: true
        use_types: true
        username: user
        password: secret
        condition: '${kubernetes_leaderelection.leader} == true'

Integration After Upgrade (with no user intervention)

inputs:
  - id: prometheus/metrics-prometheus-eb767fbc-1e20-47da-b85d-973f5b4195f2
    name: prometheus-1
    revision: 2
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.2
    data_stream:
      namespace: default
    package_policy_id: eb767fbc-1e20-47da-b85d-973f5b4195f2
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-eb767fbc-1e20-47da-b85d-973f5b4195f2
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        condition: null
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

Integration After Upgrade (with user manually re-adding Leader Election condition)

inputs:
  - id: prometheus/metrics-prometheus-eb767fbc-1e20-47da-b85d-973f5b4195f2
    name: prometheus-1
    revision: 3
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.2
    data_stream:
      namespace: default
    package_policy_id: eb767fbc-1e20-47da-b85d-973f5b4195f2
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-eb767fbc-1e20-47da-b85d-973f5b4195f2
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        condition: '${kubernetes_leaderelection.leader} == true'
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

How to test this PR locally

Spin up test stack and install Prometheus 1.1.0, enabling Leader Election
Upgrade to 1.2.0.

Related issues

Closes [Prometheus][Collector] Support Customizing condition #5259

Screenshots

elasticmachine · 2023-02-18T16:21:32Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-02-22T03:59:07.796+0000
Duration: 16 min 33 sec

Test stats 🧪

Test	Results
Failed	0
Passed	9
Skipped	0
Total	9

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

ChrsMark

Thanks for your PR!
TBH, I would like to have both options available.
The leaderelection is quite specific and is quite handy for users to easily enable the integration on a cluster scope. The generic condition you are adding is also useful but should not replace the leaderelection one. Having both is not only better for the UX but is also useful in case one user wants to add a specific condition like "k8s.label == my-app" while having the leaderelection enabled too.

BenB196 · 2023-02-20T11:52:25Z

Hi @ChrsMark understood. I thought about this being a potential option, but wasn't really sure how to implement it, Would you know if there is an existing integration that does something similar that I could use as a reference?

BenB196 · 2023-02-20T13:01:55Z

Thinking about this a bit more, I suppose something like this might work:

{{#if (or leaderelection condition)}}
condition: {{#if leaderelection}}${kubernetes_leaderelection.leader} == true{{/if}}{{#if condition}}{{ condition }}{{/if}}
{{/if}}

I can't really think of any use-cases that would require a user to set both leaderelection and a condition, but I figure if there is a case, it's probably far enough an edge enough, that a user can just prefix their condition statement with an and or or to allow the 2 to be joined.

(I haven't tested the above yet, so not 100% how it will actually work in practice)

BenB196 · 2023-02-21T00:13:24Z

Hi @ChrsMark would you be able to take a look at the change to this PR again when you have a moment and see if this better aligns with the desired state?

The hbs file becomes a bit messy with the additional logic, but testing appears to work.

Integration Before Upgrade (with Leader Election enabled)

inputs:
  - id: prometheus/metrics-prometheus-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    name: prometheus-1
    revision: 1
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.1.0
    data_stream:
      namespace: default
    package_policy_id: b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metricsets:
          - collector
        hosts:
          - 'localhost:9090'
        metrics_filters.exclude: null
        metrics_filters.include: null
        metrics_path: /metrics
        period: 10s
        rate_counters: true
        use_types: true
        username: user
        password: secret
        condition: '${kubernetes_leaderelection.leader} == true'

Integration After Upgrade

inputs:
  - id: prometheus/metrics-prometheus-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    name: prometheus-1
    revision: 2
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.0
    data_stream:
      namespace: default
    package_policy_id: b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        condition: '${kubernetes_leaderelection.leader} == true'
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

Integration After Upgrade (adding a condition)

inputs:
  - id: prometheus/metrics-prometheus-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    name: prometheus-1
    revision: 4
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.0
    data_stream:
      namespace: default
    package_policy_id: b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        condition: '${kubernetes_leaderelection.leader} == true and k8s.label == my-app'
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

Integration After Upgrade (Disabling leader election)

inputs:
  - id: prometheus/metrics-prometheus-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    name: prometheus-1
    revision: 5
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.0
    data_stream:
      namespace: default
    package_policy_id: b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        condition: k8s.label == my-app
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

Integration After Upgrade (Removing condition)

inputs:
  - id: prometheus/metrics-prometheus-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    name: prometheus-1
    revision: 6
    type: prometheus/metrics
    use_output: default
    meta:
      package:
        name: prometheus
        version: 1.2.0
    data_stream:
      namespace: default
    package_policy_id: b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
    streams:
      - id: >-
          prometheus/metrics-prometheus.collector-b7e2f2ce-8b43-4fca-a1bf-c9b9606c9b5a
        data_stream:
          dataset: prometheus.collector
          type: metrics
        metrics_filters.exclude: null
        period: 10s
        password: secret
        hosts:
          - 'localhost:9090'
        metricsets:
          - collector
        use_types: true
        metrics_filters.include: null
        rate_counters: true
        metrics_path: /metrics
        username: user

A note, after thinking about this for a bit, I've opted to include an and operator if both leaderelection and condition. I think there are some cases where you'd want leader election and additional conditions, but I wasn't able to think of any use cases where you'd want leader election or condition. I think by automatically adding an and operator, it improves the UX for most cases. In the event someone has a "real" use case where they want an or operator, they'd just need to manually set leaderelection in the conditon

ChrsMark · 2023-02-21T04:58:17Z

Hey @BenB196 ! You can find a similar PR that adds the condition setting at #4324.

I don't think it's a good idea to mix the leaderelection with the generic condition using and/or operators. The leaderelection setting should be untouched and you can just add the additional condition similarly to #4324.

So the stream.yml.hbs file should be like:

...
{{#if leaderelection}}
condition: ${kubernetes_leaderelection.leader} == true
{{/if}}
{{#if condition}}
condition: {{ condition }}
{{/if}}
...

If any of these 2 are omitted in the input the respective section will be ignored when Agent "compiles" the above file.

This means that as a user I can enable the collector data_stream only for the leader Agent and at the same point define a condition like ${kubernetes.labels.scrape} == true to apply the data_stream on Pods with specific label. That's the main usage of the conditions.

tetianakravchenko · 2023-02-21T08:55:30Z

@ChrsMark but in the case you suggested we might get condition repeated twice if leaderelection is set to true and condition defined:

condition: ${kubernetes_leaderelection.leader} == true
condition: k8s.label == my-app

I didn't test it, do you know if condition will be overwritten by the last value in configuration in this case?

ChrsMark · 2023-02-21T10:26:35Z

@tetianakravchenko yeap you are right about this, only 1 condition is accepted most probably. This means my previous comment is totally invalid, and @BenB196 please ignore it. Apologies for any confusion.

I think how it's currently shaped is fine:

{{#if leaderelection}}
  {{#if condition }}
    condition: ${kubernetes_leaderelection.leader} == true and {{ condition }}
  {{ else }}
    condition: ${kubernetes_leaderelection.leader} == true
  {{/if}}
{{ else }}
  {{#if condition }}
    condition: {{ condition }}
  {{/if}}
{{/if}}

@BenB196 if both are set by the user then the and operator is more suitable.

@tetianakravchenko wdyt?

tetianakravchenko · 2023-02-21T10:40:25Z

@ChrsMark yes, I agree, how it's currently shaped is fine.

optionally else could be omitted, but it is not required:

{{#if leaderelection}}
  {{#if condition }}
    condition: ${kubernetes_leaderelection.leader} == true and {{ condition }}
  {{ else }}
    condition: ${kubernetes_leaderelection.leader} == true
  {{/if}}
{{/if}}
{{#if condition }}
   condition: {{ condition }}
{{/if}}

packages/prometheus/data_stream/collector/manifest.yml

elasticmachine · 2023-02-22T04:14:52Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`0/0`)	💚
Files	100.0% (`0/0`)	💚 2.206
Classes	100.0% (`0/0`)	💚 2.206
Methods	88.889% (`8/9`)	👎 -2.436
Lines	100.0% (`0/0`)	💚 7.022
Conditionals	100.0% (`0/0`)	💚

ChrsMark

LGTM, I also tested this branch on my end and the generated policies for each combination look good.

elasticmachine · 2023-02-22T09:30:25Z

Package prometheus - 1.2.0 containing this change is available at https://epr.elastic.co/search?package=prometheus

* Enable setting condition on Prometheus Collector * Fixed missed update of manifest * exclude condition if null * Re-add lost change to condition required * Re-add leaderelection with condition * Fix leaderelection required

Enable setting condition on Prometheus Collector

a1e4756

Fixed missed update of manifest

2869fa6

BenB196 marked this pull request as ready for review February 18, 2023 16:38

BenB196 requested a review from a team as a code owner February 18, 2023 16:38

BenB196 requested review from gizas and MichaelKatsoulis February 18, 2023 16:38

BenB196 added 2 commits February 18, 2023 13:14

exclude condition if null

a6c2172

Re-add lost change to condition required

a0a6711

ChrsMark requested changes Feb 20, 2023

View reviewed changes

Re-add leaderelection with condition

f6e4ed0

tetianakravchenko reviewed Feb 21, 2023

View reviewed changes

packages/prometheus/data_stream/collector/manifest.yml Outdated Show resolved Hide resolved

Fix leaderelection required

93ff8fb

ChrsMark approved these changes Feb 22, 2023

View reviewed changes

tetianakravchenko approved these changes Feb 22, 2023

View reviewed changes

tetianakravchenko merged commit 07da076 into elastic:main Feb 22, 2023

BenB196 deleted the change-prometheus-collector-condition branch February 22, 2023 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting condition on Prometheus Collector #5317

Enable setting condition on Prometheus Collector #5317

BenB196 commented Feb 18, 2023 •

edited

Loading

elasticmachine commented Feb 18, 2023 •

edited

Loading

Build stats

Test stats 🧪

ChrsMark left a comment

BenB196 commented Feb 20, 2023 •

edited

Loading

BenB196 commented Feb 20, 2023

BenB196 commented Feb 21, 2023 •

edited

Loading

ChrsMark commented Feb 21, 2023 •

edited

Loading

tetianakravchenko commented Feb 21, 2023

ChrsMark commented Feb 21, 2023 •

edited

Loading

tetianakravchenko commented Feb 21, 2023 •

edited

Loading

elasticmachine commented Feb 22, 2023

ChrsMark left a comment

elasticmachine commented Feb 22, 2023

Enable setting condition on Prometheus Collector #5317

Enable setting condition on Prometheus Collector #5317

Conversation

BenB196 commented Feb 18, 2023 • edited Loading

What does this PR do?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Screenshots

elasticmachine commented Feb 18, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

ChrsMark left a comment

Choose a reason for hiding this comment

BenB196 commented Feb 20, 2023 • edited Loading

BenB196 commented Feb 20, 2023

BenB196 commented Feb 21, 2023 • edited Loading

ChrsMark commented Feb 21, 2023 • edited Loading

tetianakravchenko commented Feb 21, 2023

ChrsMark commented Feb 21, 2023 • edited Loading

tetianakravchenko commented Feb 21, 2023 • edited Loading

elasticmachine commented Feb 22, 2023

🌐 Coverage report

ChrsMark left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 22, 2023

BenB196 commented Feb 18, 2023 •

edited

Loading

elasticmachine commented Feb 18, 2023 •

edited

Loading

BenB196 commented Feb 20, 2023 •

edited

Loading

BenB196 commented Feb 21, 2023 •

edited

Loading

ChrsMark commented Feb 21, 2023 •

edited

Loading

ChrsMark commented Feb 21, 2023 •

edited

Loading

tetianakravchenko commented Feb 21, 2023 •

edited

Loading